Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace dismiss button in Flash with IconButton #2222

Merged
merged 5 commits into from
Aug 25, 2023
Merged

Replace dismiss button in Flash with IconButton #2222

merged 5 commits into from
Aug 25, 2023

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Aug 22, 2023 •
edited
Loading

What are you trying to accomplish?

Fixing https://github.com/github/primer/issues/2550 by replacing the custom dismiss button with a PVC IconButton

Screenshots

Visual Diffs

Integration

No

List the issues that this change affects.

Closes https://github.com/github/primer/issues/2550

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

There was some padding adjustment I did for the position of the button.

Anything you want to highlight for special attention from reviewers?

Accessibility

  • Fixes axe scan violation - This change fixes an existing axe scan violation.
  • No new axe scan violation - This change does not introduce any new axe scan violations.
  • New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@jonrohan jonrohan requested a review from a team as a code owner August 22, 2023 20:53
@jonrohan jonrohan requested review from a team, langermank and keithamus August 22, 2023 20:53
@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 6725c1d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added ruby Pull requests that update Ruby code css Pull requests that update CSS code labels Aug 22, 2023
@github-actions
Copy link
Contributor

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

@jonrohan
Copy link
Member Author

I want to get @maximedegreve's review before merging this

@langermank
Copy link
Contributor

Hmm, can you fix the deploy so we can see it in Lookbook?

@keithamus keithamus temporarily deployed to preview August 23, 2023 10:36 — with GitHub Actions Inactive
@jonrohan jonrohan temporarily deployed to preview August 23, 2023 15:57 — with GitHub Actions Inactive
@jonrohan jonrohan merged commit e196af7 into main Aug 25, 2023
27 checks passed
@jonrohan jonrohan deleted the flash_replace_dismiss branch August 25, 2023 17:55
@jonrohan jonrohan temporarily deployed to preview August 25, 2023 17:55 — with GitHub Actions Inactive
@primer primer bot mentioned this pull request Aug 25, 2023
Merged
@maximedegreve
Copy link
Contributor

Looks good. Thank you for fixing this @jonrohan

@openprojectci openprojectci mentioned this pull request Sep 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithamus keithamus keithamus approved these changes

@camertron camertron camertron approved these changes

@langermank langermank langermank approved these changes

@maximedegreve maximedegreve Awaiting requested review from maximedegreve

Assignees
No one assigned
Labels
css Pull requests that update CSS code minor release ruby Pull requests that update Ruby code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jonrohan @langermank @maximedegreve @keithamus @camertron